Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0] remove popover #17071

Merged
merged 65 commits into from
Jul 28, 2017
Merged

[4.0] remove popover #17071

merged 65 commits into from
Jul 28, 2017

Conversation

brianteeman
Copy link
Contributor

@brianteeman brianteeman commented Jul 11, 2017

This is a pr for #17044 to remove the redundant popovers.

PURELY for the purpose of testing this concept I have quickly displayed the remaining popover as inline text - THIS PART IS NOT FOR REVIEW - it is just to demonstrate how few remaining descriptions we will have

In most cases the description could be simply removed
In some cases by adjusting the label the description could be removed.
Of the remaining descriptions some have been updated to remove redundancy and duplication but they need further review/testing but that is not part of this pr

Strings that are no longer used have been deleted unless they are in a global language file

References to labels and descriptions in Filters have been removed as they were neither being used not did 99% of them even exist. NOTE I did not look at the filters in /components as i could not see where or if they are even being used

This is a proof of concept for joomla#17044 to remove the redundant popovers.

PURELY for the purpose of testing this concept I have quickly displayed the remaining popover as inline text - THIS PART IS NOT FOR REVIEW - it is just to demonstrate how few remaining descriptions we will have

In addition you will notice that the remainining descriptions are not very good. They are manuals not tips, hints or descritpions and can be drastically reduced or rewritten. AGAIN this text is not for review here
@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-4.0-dev labels Jul 11, 2017
@brianteeman brianteeman requested a review from wilsonge July 11, 2017 12:04
@C-Lodder
Copy link
Member

+1

COM_BANNERS_FIELD_ALT_LABEL="Alt Text"
COM_BANNERS_FIELD_BANNEROWNPREFIX_DESC="Use own prefix or the client prefix."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK So with switch removing this becomes not possible. There's nothing stating that off means client prefix (nor is it obvious)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good example of where the existing label is simply wrong

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then let's fix the label now ;) We can't remove the description if it's the only thing explaining what's going on :)

@@ -69,73 +69,40 @@ COM_BANNERS_ERR_ZIP_CREATE_FAILURE="Zip create failure"
COM_BANNERS_ERR_ZIP_DELETE_FAILURE="Zip delete failure"
COM_BANNERS_ERROR_UNIQUE_ALIAS="Another Banner from this category has the same alias (remember it may be a trashed item)."
COM_BANNERS_EXTRA="Additional Information"
COM_BANNERS_FIELD_ALIAS_DESC="The alias is for internal use only. Leave this blank and Joomla will fill in a default value from the title. It has to be unique for each banner in the same category."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has to be unique for each banner in the same category does this not need to be stated somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we need to state it. In most cases it it will be automatically generated and if someone does it manually and uses an existing alias then they will get an error message

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have a clue what it does then I would be happy to but I have zero clue

COM_BANNERS_FIELD_IMPMADE_LABEL="Total Impressions"
COM_BANNERS_FIELD_IMPTOTAL_DESC="Total limit of impressions defined for the banner."
COM_BANNERS_FIELD_IMPTOTAL_LABEL="Max. Impressions"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is max the same as total limit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again an example of existing unclear labels

Max Impressions is the limit
Total Impressions is the current count

@wilsonge
Copy link
Contributor

Just gone through the list of strings (haven't matched them to the XML files). Largely this looks ok although there's a couple where I'm a bit uncertain

@brianteeman brianteeman changed the title [4.0] com_banners remove popover [4.0] com_banners/contacts remove popover Jul 13, 2017
@joomla-cms-bot joomla-cms-bot removed the Language Change This is for Translators label Jul 17, 2017
@brianteeman
Copy link
Contributor Author

ok i am done. i have updated the original post and this is ready for review. Please read the notes carefully

@joomla-cms-bot joomla-cms-bot removed this from the Joomla 4.0 milestone Jul 26, 2017
@dgrammatiko
Copy link
Contributor

RTC @wilsonge please merge this to proceed with our road map

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 26, 2017
@brianteeman
Copy link
Contributor Author

From what I can tell the drone issue is unrelated

@dgrammatiko
Copy link
Contributor

@brianteeman it is unrelated and it was patched in couple PRs that never made it to the core

@wilsonge
Copy link
Contributor

If you get conflicts done here I'll get it merged

@brianteeman
Copy link
Contributor Author

What again :(
I will do it in the morning and hopefully it will be !edged before any more

@wilsonge
Copy link
Contributor

It's the PR of your's I merged this morning :) I'll get this merged next!

@brianteeman
Copy link
Contributor Author

Yes I know.i am just pulling your chain

@brianteeman
Copy link
Contributor Author

Conflicts resolved - hopefully for the last time.

@wilsonge

@wilsonge wilsonge merged commit e4b9c4d into joomla:4.0-dev Jul 28, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 28, 2017
@wilsonge
Copy link
Contributor

Conflicts resolved - hopefully for the last time

Only for this PR I'm afraid ;)

@wilsonge wilsonge added this to the Joomla 4.0 milestone Jul 28, 2017
@brianteeman
Copy link
Contributor Author

Thanks

@brianteeman brianteeman deleted the com_banners branch July 28, 2017 09:27
heelc29 added a commit to heelc29/joomla that referenced this pull request Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants